[WiP] Selecting multiple files for deleting on left menu #368

guergana · 2024-05-05T15:31:45Z

fixes Problem selecting multiple files on the left menu #341

cloudflare-workers-and-pages · 2024-05-06T15:10:09Z

Deploying opendataeditor with Cloudflare Pages

Latest commit:	`f0a32f0`
Status:	✅ Deploy successful!
Preview URL:	https://75fad6b3.opendataeditor.pages.dev
Branch Preview URL:	https://341-selecting-multiple-files.opendataeditor.pages.dev

View logs

guergana · 2024-06-02T11:57:39Z

client/components/Application/Dialogs/MoveFile.tsx

  return (
    <InputDialog
      open={true}
-      value={path}
+      value={paths[0]}


Not sure how moving file works. This might need to be reworked as well for multiple files

guergana · 2024-06-02T11:58:25Z

client/components/Application/Dialogs/MoveFolder.tsx

  return (
    <InputDialog
      open={true}
-      value={path}
+      value={paths[0]}


Same as with moving files. This might need to be adjusted for multiSelection

guergana · 2024-06-02T11:59:32Z

client/components/Application/store.ts

+      for (const path of paths) {
+        await client.fileDelete({ path })
+        onFileDelete(path)
+      }
    },
    moveFile: async (path, toPath) => {
      const { client, onFileCreate } = get()
      const result = await client.fileMove({ path, toPath, deduplicate: true })


I don't know how this works. haven't tested it yet

guergana · 2024-06-02T12:00:44Z

client/components/Application/store.ts

+      if (paths === newPaths) return
+      set({ paths: newPaths })
+      if (!newPaths) return
+      if (record?.path === newPaths[0]) return


not sure what this is doing. using only the first path just in case. Is it for the history? Do we need to modify the record to add all the selected paths?

guergana · 2024-06-02T12:02:45Z

client/components/Application/store.ts

      if (selectors.isFolder(get())) return
-      await openFile(newPath)
+      // if more than one file is selected, display the only the first one
+      if (paths && paths[0] !== newPaths[0]) await openFile(newPaths[0])


This is still not working 😅

guergana · 2024-06-07T10:01:49Z

@roll I need your help with this PR. I have some issues I can't figure out on my own, besides the fact that I am changing parts of the application in this ticket that I am not sure which side effects they could have..

I don't understand why here: https://github.com/okfn/opendataeditor/blob/main/client/components/Application/Content.tsx#L28 ? the FileContent is updated and fired even if the client and the record stay the same. If you test, when you select one File in the left menu it loads correctly, but when you select a second file, in theory, the FileContent component should stay the same, but it updates and returns a blank screen. Could you help me debug and test since you are the most familiar with all parts of the application? 🙏

roll · 2024-06-10T09:11:39Z

@guergana
@pdelboca
As we don't yet have tests for this store, and the complexity is already very high, I think the safest approach here would be to introduce a completely new state property like secondaryPaths with completely separated logic that only works for selection and deleting (the user story requirement), for example:

on file click we set path='file1'
on ctrl-click on other file we set secondaryPaths=['file2']
[handling selecting / deselecting / other logic ]
on "Delete" click we just delete path and all the secondaryPaths

I think this kind of implementation at least won't break any existent functionality. WDYT?

guergana · 2024-06-10T11:27:04Z

@guergana @pdelboca As we don't yet have tests for this store, and the complexity is already very high, I think the safest approach here would be to introduce a completely new state property like secondaryPaths with completely separated logic that only works for selection and deleting (the user story requirement), for example:
* on file click we set `path='file1'`

* on ctrl-click on other file we set `secondaryPaths=['file2']`

* [handling selecting / deselecting / other logic ]

* on "Delete" click we just delete `path` and all the `secondaryPaths`
I think this kind of implementation at least won't break any existent functionality. WDYT?

@roll I thought about doing something similar like paths when it was a multiselect and only path for single select, but then also thought this is not a very elegant solution... this is why i went for changing the whole store, but yes, as you point out, this is a very risky approach since we don't have tests throught the application and i might have broken something. I agree your approach is a good solution for now.

guergana · 2024-06-14T08:41:51Z

@guergana @pdelboca As we don't yet have tests for this store, and the complexity is already very high, I think the safest approach here would be to introduce a completely new state property like secondaryPaths with completely separated logic that only works for selection and deleting (the user story requirement), for example:
* on file click we set `path='file1'`

* on ctrl-click on other file we set `secondaryPaths=['file2']`

* [handling selecting / deselecting / other logic ]

* on "Delete" click we just delete `path` and all the `secondaryPaths`
I think this kind of implementation at least won't break any existent functionality. WDYT?

@roll I am trying to implement your proposed approach but I think it will also complicate the logic. I can't find a way to implement this in a simple way. Would you mind taking over this task and implementing your proposed approach?

guergana · 2024-06-14T19:51:54Z

Finally, @pdelboca s proposed approach worked better and I have implemented it in a different PR. I will close this PR in favor of #426

guergana marked this pull request as draft May 5, 2024 15:31

guergana added 4 commits May 31, 2024 15:48

Selecting multiple files for deleting on left menu

044516e

Make path state an array

f2c30d7

Adjust store to handle a paths array instead of path as string

af2f975

Fix all the files affected by changing paths to be an array

2b23a80

guergana force-pushed the 341-selecting-multiple-files-menu branch from 9f0f583 to 2b23a80 Compare June 2, 2024 11:15

Fixes

d0cabb9

guergana commented Jun 2, 2024

View reviewed changes

guergana commented Jun 3, 2024

View reviewed changes

Test, still not working

f0a32f0

guergana requested a review from roll June 7, 2024 10:01

guergana closed this Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WiP] Selecting multiple files for deleting on left menu #368

[WiP] Selecting multiple files for deleting on left menu #368

guergana commented May 5, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 6, 2024 •

edited

Loading

guergana Jun 2, 2024

guergana Jun 2, 2024

guergana Jun 2, 2024

guergana Jun 2, 2024

guergana Jun 2, 2024

guergana commented Jun 7, 2024

roll commented Jun 10, 2024

guergana commented Jun 10, 2024

guergana commented Jun 14, 2024

guergana commented Jun 14, 2024

[WiP] Selecting multiple files for deleting on left menu #368

[WiP] Selecting multiple files for deleting on left menu #368

Conversation

guergana commented May 5, 2024 • edited Loading

cloudflare-workers-and-pages bot commented May 6, 2024 • edited Loading

Deploying opendataeditor with Cloudflare Pages

guergana Jun 2, 2024

Choose a reason for hiding this comment

guergana Jun 2, 2024

Choose a reason for hiding this comment

guergana Jun 2, 2024

Choose a reason for hiding this comment

guergana Jun 2, 2024

Choose a reason for hiding this comment

guergana Jun 2, 2024

Choose a reason for hiding this comment

guergana commented Jun 7, 2024

roll commented Jun 10, 2024

guergana commented Jun 10, 2024

guergana commented Jun 14, 2024

guergana commented Jun 14, 2024

guergana commented May 5, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 6, 2024 •

edited

Loading